Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pickling methods to support TorchScript. #74

Merged
merged 3 commits into from
Oct 31, 2023
Merged

Conversation

HapeMask
Copy link
Contributor

Sphericart already uses TORCH_LIBRARY but when serializing the library reference into a TorchScript JIT module, the class needs a pickle/unpickle method as well.

@Luthaf
Copy link
Contributor

Luthaf commented Oct 26, 2023

Thank you for doing this! The code looks good but fails the formatting test. Could you run clang-format on the files you modified? Then everything should be good to go!

@HapeMask
Copy link
Contributor Author

Could you run clang-format on the files you modified?

Sure! I tried this quickly and it reformatted the entire file, which probably means I did something wrong. I'll look into it shortly.

@frostedoyster
Copy link
Collaborator

Thanks for the PR, @HapeMask! Could you share with us the exact clang-format command you ran (and also the directory you ran it from)? We're trying to improve our C++ formatting, perhaps I can open a PR for that

@HapeMask
Copy link
Contributor Author

Thanks for the PR, @HapeMask! Could you share with us the exact clang-format command you ran (and also the directory you ran it from)? We're trying to improve our C++ formatting, perhaps I can open a PR for that

Yup, here's some info on what I ran + version:

]$ clang-format --version
clang-format version 16.0.1 (https://github.com/conda-forge/clangdev-feedstock 1dd6d282bc8be5828cf8c163d2c68cc525f527c5)

]$ clang-format -i sphericart-torch/src/torch.cpp
]$ git log
commit 2f07d5f (HEAD -> pickle, mine/pickle)
Author: Gabe Schwartz <[email protected]>
Date:   Wed Oct 25 15:52:31 2023 -0700

    Add pickling methods to support TorchScript.

commit 8ffaf38 (origin/main, origin/HEAD, main)
Author: Filippo Bigi <[email protected]>
Date:   Fri Aug 18 14:20:46 2023 +0200

    Fix computations for large output sizes (#67)

    * Fix large-size integer overflows on CPU
    * Allow computations on large tensors on GPU

    ---------

    Co-authored-by: Nick J. Browning <[email protected]>
    Co-authored-by: Guillaume Fraux <[email protected]>

and this is the changeset it produced for that file: https://gist.github.com/HapeMask/31d530a691b0937344d7392eee8cc0e0

I'm happy to just copy/paste the formatting changes to only the code I added, just let me know.

@frostedoyster
Copy link
Collaborator

I tested clang-format-10 and clang-format-17 on main and they give the exact same results, so I would guess that version is not the issue. From the changelog that @HapeMask sent, it seems that the first indent is 2 spaces, while all others are 4 spaces. It's also reordering the includes (alphabetically) and it seems to dislike long lines. Almost as if it was operating according to a different set of formatting instructions. @Luthaf any ideas? What should we do?

@Luthaf
Copy link
Contributor

Luthaf commented Oct 30, 2023

What happens if you run clang-format --style=file:$(pwd)/.clang-format -i sphericart-torch/src/torch.cpp? I though this was the default behavior, but let's check!

@HapeMask
Copy link
Contributor Author

HapeMask commented Oct 30, 2023

Oh! I don't know how this happened but my clone of your main branch was from Aug. 18th (before the .clang-format file was added to the repo), so I didn't have that file in my branch. Re-running after merging the latest main branch gives better results, I'll push the changes today.

@Luthaf Luthaf merged commit 1dc5338 into lab-cosmo:main Oct 31, 2023
10 checks passed
@Luthaf
Copy link
Contributor

Luthaf commented Oct 31, 2023

Thanks @HapeMask!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants